Improve: allow PreSaveEvent listeners to skip default object persistence - #607
Conversation
Add a skip-save flag to PreSaveEvent so listeners can decide programmatically whether an imported row should be persisted. ImportProcessingService now checks this flag after dispatching PreSaveEvent and returns early when save is skipped. Default behavior remains unchanged unless explicitly requested by a listener.
|
|
Hi @fashxp, could you give me your opinion on the event extension proposal? It would be very helpful to me. If you think it's a good idea, I can reduce the cognitive complexity flagged by Sonar by moving the "if" and the two lines of code above into a new method. However, I think it’s outside of my responsibility, don’t I? |
|
- add upgrade note for 2026.3.0 and document the new opt-out in the events docs - write an info log entry when a listener skips the save, so skipped rows stay traceable in the import log - declare $skipSave as a typed private property (the class is final)
kingjia90
left a comment
kingjia90
left a comment
There was a problem hiding this comment.
Reviewed the change end to end — it does what pimcore/platform-version#303 asks for, and it does it in the right place.
Behaviour
processElement() dispatches PreSaveEvent inside the try { … } finally { … } that restores Version::$disabled, so the early return still runs the finally and cannot leak a disabled versioning state into the next record — verified in context. Skipping also means no checkKey(), no save() and no PostSaveEvent, which is the correct semantics for "this element was not persisted". The queue item is still consumed by the caller, so a skipped row does not stall or retry.
BC
Purely additive: a new flag defaulting to false on an existing event, plus one guard clause. No existing listener, resolver or import configuration changes behaviour.
What I added on top (401c084)
- Upgrade note for 2026.3.0 and documentation of the opt-out in
doc/06_Extending/02_Events.md, including an example listener. - An info log entry when a listener skips the save. Without it a skipped record produced no log line at all — neither the success message nor an error — so rows would silently disappear from an import run. The import log is the primary debugging surface here, so a skip should be visible in it.
private bool $skipSave = false;instead of the untypedprotected $skipSave. The class isfinal, and every other property on the event hierarchy is typed.
Also merged current 2026.x into the branch, since the documentation tree was restructured after this PR was opened.
No test: ImportProcessingService has no unit-test harness in this bundle (it needs a resolver, mapping configuration, dispatcher and application logger), and a test that only exercises the event's getter/setter would not cover the guard clause. Flagging it rather than hiding it.
Good to merge from my side.
There was a problem hiding this comment.
🟡 Changes recommended
Skipped mutations may remain cached and be persisted by a later row.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds listener-controlled skipping of default DataObject persistence during imports.
Changes:
- Adds a default-false skip flag to
PreSaveEvent. - Checks the flag before saving and logs skipped persistence.
- Documents the event behavior and upgrade note.
Review contract:
- Intent/root cause: Directly adds the missing event control (
PreSaveEvent.php:20-29). - Call sites/boundary: The persistence owner checks the flag at the sole dispatch site (
ImportProcessingService.php:241-256). - Compatibility: Additive API with unchanged default behavior.
- Tests: No regression coverage accompanies the new branch.
- Docs: Event and upgrade documentation were updated.
- Risk: Skipped mutations can remain in Pimcore’s runtime-cached object and leak into later rows (
ImportProcessingService.php:244-250).
File summaries
| File | Description |
|---|---|
src/Processing/ImportProcessingService.php |
Skips persistence when requested. |
src/Event/DataObject/PreSaveEvent.php |
Adds the mutable skip flag API. |
doc/06_Extending/02_Events.md |
Documents usage and behavior. |
doc/01_Installation/01_Upgrade.md |
Records the feature and compatibility. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ($event->shouldSkipSave()) { | ||
| $this->logInfo($configName, 'Saving of element skipped by PreSaveEvent listener.', [ | ||
| 'component' => PimcoreDataImporterBundle::LOGGER_COMPONENT_PREFIX . $configName, | ||
| 'relatedObject' => $element | ||
| ]); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Good catch — confirmed and fixed in f1666de.
loadOrCreateAndPrepareElement() resolves through DataObject::getById()/getByPath(), which return the runtime-cached instance, and both the sequential command and the Messenger handler process many queue items in one process. Before the skip flag this was harmless because every resolved element was saved, so the in-memory state matched the database. With a skip it no longer does, and a later row resolving the same element would have persisted the skipped row's values.
The skip path now calls discardUnsavedChanges(), which reloads an existing element through Element\Service::getElementById($type, $id, ['force' => true]). The forced load re-registers a clean instance in the runtime cache, so the next row resolving the same element gets database state. Newly created elements have no ID and were never cached, so they are simply dropped.
The extra load is paid only on skipped rows that resolved an existing element, and it usually hits the Pimcore item cache rather than the database. Documented in the upgrade note and in doc/06_Extending/02_Events.md.
…ter row Pimcore's loaders hand out runtime-cached instances, so the mapping changes applied to a skipped element would otherwise be inherited - and saved - by a later row resolving the same element.
|
Summary
This PR introduces a small improvement to the Data Importer flow:
listeners of
PreSaveEventcan now programmatically decide whether the current imported object should be persisted or not.Motivation
In some import scenarios, a listener may fully handle a row (e.g. custom processing, aggregation, side effects) and therefore the default save operation should be skipped.
Before this change,
PreSaveEventlisteners could mutate data but could not explicitly prevent persistence.What changed
PreSaveEventto mark save as skippable.shouldSkipSave()/setSkipSave(bool $skipSave)).PreSaveEventand return early when save should be skipped.Result
This enables extension points where listeners can:
Backward compatibility
false, so existing imports continue to save as before.Closes pimcore/platform-version#303